feat(mail): persist inbound attachment bytes to BlobStore (HT-46) - #47
Merged
Conversation
📝 WalkthroughWalkthroughInbound thread attachments are persisted in BlobStore with transactional database references, then optionally exposed in Agent Inbox conversation responses as per-thread metadata with signed URLs. Ingestion, retry behavior, migrations, storage, API wiring, specifications, and tests are updated. ChangesInbound thread attachments
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MailIngest
participant BlobStore
participant ConversationStore
participant ThreadAttachmentStore
participant AgentInboxAPI
MailIngest->>BlobStore: write attachment bytes
MailIngest->>ConversationStore: persist thread and delivery transaction
MailIngest->>ThreadAttachmentStore: insert blob-key references in step-five transaction
AgentInboxAPI->>ThreadAttachmentStore: list attachments by conversation
AgentInboxAPI->>BlobStore: mint one-hour signed URLs
AgentInboxAPI-->>AgentInboxAPI: return attachments per thread
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/conversations.ts`:
- Around line 271-280: Update the attachment processing in the conversation
handler around toAttachmentViewJson to resolve all attachment views concurrently
with Promise.all, then iterate over the ordered results to populate byThreadId.
Preserve the existing grouping behavior and attachment order while avoiding
sequential BlobStore calls.
In `@src/api/index.test.ts`:
- Line 527: Remove the duplicated test declarations in src/api/index.test.ts: at
lines 527-527, retain only one const body = await res.json() declaration; at
lines 600-600, retain only one threads property in the response type. No other
test behavior needs to change.
In `@src/mail/ingest.ts`:
- Around line 399-417: Update sanitizeAttachmentFilename to map complete "." and
".." results to the existing "attachment" placeholder, while preserving the
current sanitization for all other filenames and the existing handling of null,
undefined, and empty values. Add regression coverage asserting both dot-segment
inputs return "attachment".
In `@src/store/attachments.test.ts`:
- Around line 113-122: Update the test around listByConversationId to preserve
and verify the store’s ordering rather than sorting the returned rows. Make the
two inserts use distinct timestamps so the expected ordering is unambiguous,
then assert the returned filenames directly in the sequence produced by
listByConversationId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc2fe694-9b5b-4dff-ab40-58ecb14840a1
📒 Files selected for processing (14)
specs/api/agent-inbox-v1.mdspecs/mail/inbound-ingestion.mdsrc/api/conversations.tssrc/api/index.test.tssrc/api/index.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/store/attachments.test.tssrc/store/attachments.tssrc/store/index.ts
| const { conversationId } = await store.createConversation(newConversation()) | ||
|
|
||
| const res = await api(get(`/api/v1/conversations/${conversationId}`)) | ||
| const body = (await res.json()) as { threads: Array<{ attachments: unknown[] }> } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove accidentally duplicated test source. These duplicate declarations prevent clean typechecking/compilation.
src/api/index.test.ts#L527-L527: retain only oneconst body = await res.json()declaration.src/api/index.test.ts#L600-L600: retain only onethreadsproperty in the response type.
📍 Affects 1 file
src/api/index.test.ts#L527-L527(this comment)src/api/index.test.ts#L600-L600
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/index.test.ts` at line 527, Remove the duplicated test declarations
in src/api/index.test.ts: at lines 527-527, retain only one const body = await
res.json() declaration; at lines 600-600, retain only one threads property in
the response type. No other test behavior needs to change.
Comment on lines
+399
to
+417
| /** | ||
| * The filename segment of an attachment's blob key — NOT the `filename` | ||
| * column value (that stays the original, verbatim `ParsedAttachment.filename`, | ||
| * `null` included). `BlobStore` implementations (e.g. Supabase Storage, | ||
| * `src/providers/adapters/supabase-storage/`) reject object keys containing | ||
| * anything outside a restricted ASCII allowlist (letters, digits, `_`, `.`, | ||
| * `-`) — no unicode, no `/` (a path separator, which would otherwise let an | ||
| * attacker- or client-supplied filename nest the object under an unintended | ||
| * "folder" inside this attachment's own namespace slot), no `%`/`#`/quotes/ | ||
| * control characters. Every other character is replaced with `_` so the key | ||
| * stays exactly three segments deep and adapter-valid, whatever the filename | ||
| * contains. A missing OR empty filename (`null`, `undefined`, or `''` — a | ||
| * blank `''` is not caught by `??`) falls back to a fixed placeholder — the | ||
| * key still needs SOME non-empty final segment. | ||
| */ | ||
| export function sanitizeAttachmentFilename(filename: string | null): string { | ||
| const sanitized = (filename ?? '').replaceAll(/[^A-Za-z0-9._-]/g, '_') | ||
| return sanitized === '' ? 'attachment' : sanitized | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject complete . and .. filename segments.
The allowlist leaves these names unchanged, but URI processors treat complete dot segments specially, so a storage adapter or signed URL may normalize or reject the resulting key. (rfc-editor.org)
Proposed fix and regression cases
export function sanitizeAttachmentFilename(filename: string | null): string {
const sanitized = (filename ?? '').replaceAll(/[^A-Za-z0-9._-]/g, '_')
- return sanitized === '' ? 'attachment' : sanitized
+ return sanitized === '' || sanitized === '.' || sanitized === '..'
+ ? 'attachment'
+ : sanitized
}Also assert that both . and .. produce attachment.
As per coding guidelines, use RFCs and public specifications as the primary source for semantics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * The filename segment of an attachment's blob key — NOT the `filename` | |
| * column value (that stays the original, verbatim `ParsedAttachment.filename`, | |
| * `null` included). `BlobStore` implementations (e.g. Supabase Storage, | |
| * `src/providers/adapters/supabase-storage/`) reject object keys containing | |
| * anything outside a restricted ASCII allowlist (letters, digits, `_`, `.`, | |
| * `-`) — no unicode, no `/` (a path separator, which would otherwise let an | |
| * attacker- or client-supplied filename nest the object under an unintended | |
| * "folder" inside this attachment's own namespace slot), no `%`/`#`/quotes/ | |
| * control characters. Every other character is replaced with `_` so the key | |
| * stays exactly three segments deep and adapter-valid, whatever the filename | |
| * contains. A missing OR empty filename (`null`, `undefined`, or `''` — a | |
| * blank `''` is not caught by `??`) falls back to a fixed placeholder — the | |
| * key still needs SOME non-empty final segment. | |
| */ | |
| export function sanitizeAttachmentFilename(filename: string | null): string { | |
| const sanitized = (filename ?? '').replaceAll(/[^A-Za-z0-9._-]/g, '_') | |
| return sanitized === '' ? 'attachment' : sanitized | |
| } | |
| /** | |
| * The filename segment of an attachment's blob key — NOT the `filename` | |
| * column value (that stays the original, verbatim `ParsedAttachment.filename`, | |
| * `null` included). `BlobStore` implementations (e.g. Supabase Storage, | |
| * `src/providers/adapters/supabase-storage/`) reject object keys containing | |
| * anything outside a restricted ASCII allowlist (letters, digits, `_`, `.`, | |
| * `-`) — no unicode, no `/` (a path separator, which would otherwise let an | |
| * attacker- or client-supplied filename nest the object under an unintended | |
| * "folder" inside this attachment's own namespace slot), no `%`/`#`/quotes/ | |
| * control characters. Every other character is replaced with `_` so the key | |
| * stays exactly three segments deep and adapter-valid, whatever the filename | |
| * contains. A missing OR empty filename (`null`, `undefined`, or `''` — a | |
| * blank `''` is not caught by `??`) falls back to a fixed placeholder — the | |
| * key still needs SOME non-empty final segment. | |
| */ | |
| export function sanitizeAttachmentFilename(filename: string | null): string { | |
| const sanitized = (filename ?? '').replaceAll(/[^A-Za-z0-9._-]/g, '_') | |
| return sanitized === '' || sanitized === '.' || sanitized === '..' | |
| ? 'attachment' | |
| : sanitized | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/ingest.ts` around lines 399 - 417, Update sanitizeAttachmentFilename
to map complete "." and ".." results to the existing "attachment" placeholder,
while preserving the current sanitization for all other filenames and the
existing handling of null, undefined, and empty values. Add regression coverage
asserting both dot-segment inputs return "attachment".
Comment on lines
+113
to
+122
| // Both inserts above ran inside the SAME transaction, so `created_at` | ||
| // (bound to that transaction's `now()`) ties for both rows — the `id` | ||
| // tiebreak then decides order, which is not insertion order. Sort by | ||
| // filename before asserting so this test doesn't depend on that tie's | ||
| // resolution. | ||
| const rows = (await attachmentStore.listByConversationId(conversationId)).sort((a, b) => | ||
| (a.filename ?? '').localeCompare(b.filename ?? ''), | ||
| ) | ||
| expect(rows).toHaveLength(2) | ||
| expect(rows.map((r) => r.filename)).toEqual(['a.txt', 'b.png']) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the store’s ordering instead of sorting it away.
This test would still pass if listByConversationId returned attachments in the wrong order. Insert rows with distinct timestamps, then assert the returned sequence directly.
As per coding guidelines, “Convert vague requests into verifiable success criteria, preferably beginning with a failing test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/attachments.test.ts` around lines 113 - 122, Update the test around
listByConversationId to preserve and verify the store’s ordering rather than
sorting the returned rows. Make the two inserts use distinct timestamps so the
expected ordering is unambiguous, then assert the returned filenames directly in
the sequence produced by listByConversationId.
Source: Coding guidelines
Contributor
Author
Adversarial review + re-gate (orchestrator)Independent gate (re-run from a clean checkout of this branch, after pushing 1 pending local commit): typecheck 0, lint 0, tests 0 (exit codes), clean tree.
(Note: an earlier gate attempt showed ~40 unrelated tests failing with Adversarial review of record: 3 findings, all actionable, fixed and re-gated
Two other things were investigated and cleared as non-issues (noted for the record, not fixed):
All three fixes are pushed to this branch ( 🤖 Generated with Claude Code |
Writes each attachment's bytes to the BlobStore under a mailbox-namespaced
key (<mailboxId>/<attachmentId>/<filename>) BEFORE the ingest pipeline's
step-5 transaction opens, then persists only the blob-key reference inside
that transaction (new thread_attachments table, migration 015). A step-5
abort after a successful blob write leaves that blob orphaned and
unreferenced — the failure mode spec §4 already blessed — and a retry
writes a fresh blob rather than reusing or repairing the orphan.
Also wires an optional attachment read path into the Agent Inbox API:
GET /api/v1/conversations/{id}'s ThreadView now carries `attachments`
(metadata + a BlobStore signed URL), absent-by-default like open tracking
so no existing deployment or test is affected unless the composition root
opts in (this ticket wires it in for the RIQ dogfood).
Follow-up not built here: a GC sweep for orphaned blobs left behind by
aborted ingest attempts (tolerable per the ticket's design, cross-
referenced against thread_attachments in a future pass).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st (HT-46) sanitizeAttachmentFilename only stripped '/' and '\\', so any unicode, '%', '#', quote, or control character in an inbound attachment's filename produced a Supabase Storage key the adapter's server-side validation rejects on every attempt — dead-lettering the whole delivery (body included) after MAX_INGEST_ATTEMPTS. Switch to an allowlist (letters, digits, '_', '.', '-') and treat '' the same as null, since '' ?? 'attachment' let an empty filename attribute through unchanged and produced a key with an empty final segment. Also move ATTACHMENT_SIGNED_URL_EXPIRY_SECONDS above handleGetConversation's doc comment (it had been inserted between the comment and the function, orphaning the doc), and add direct unit coverage for sanitizeAttachmentFilename plus an ingest-level test proving a hostile filename still produces a valid three-segment blob key end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sistence) - Fix a stray raw NUL byte embedded in src/mail/ingest.test.ts's sanitizeAttachmentFilename control-character test — invisible in most editors/diffs, and enough to make the file read as binary to grep tools that skip binary files by default. Replaced with an explicit \x00 escape. - Parallelize signed-URL minting in attachmentViewsByThreadId (src/api/conversations.ts) — was awaiting BlobStore.getSignedUrl one attachment at a time in a loop; now Promise.all across independent calls. - Cap the sanitized attachment filename segment's length (src/mail/ingest.ts) — an attacker-controlled Content-Disposition filename has no length limit of its own, and an oversized blob-key segment would otherwise fail the same way on every retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-46-attachment-blob-persistence
branch
from
July 16, 2026 22:12
24e587c to
36235b9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mail/ingest.test.ts (1)
768-783: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a boundary assertion for the 200-character filename cap.
The hostile-input battery does not verify the explicit HT-46 length limit, so removing or miscounting the cap would pass this suite.
Proposed test
it('every result matches the adapter-safe charset and is non-empty, for a battery of hostile inputs', () => { for (const filename of [ null, '', '/', '\\', '///', 'Résumé.pdf', 'a/b/../c.txt', '文件.txt', ]) { const sanitized = sanitizeAttachmentFilename(filename) expect(sanitized.length).toBeGreaterThan(0) + expect(sanitized.length).toBeLessThanOrEqual(200) expect(sanitized).toMatch(ADAPTER_SAFE) } + + expect(sanitizeAttachmentFilename('a'.repeat(201))).toHaveLength(200) })As per coding guidelines, “Convert vague requests into verifiable success criteria, preferably beginning with a failing test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/ingest.test.ts` around lines 768 - 783, Add a focused boundary assertion in the hostile-input test around sanitizeAttachmentFilename: verify a filename longer than 200 characters is sanitized to exactly 200 characters, and verify the 200-character boundary remains accepted. Preserve the existing non-empty and ADAPTER_SAFE assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mail/ingest.test.ts`:
- Around line 768-783: Add a focused boundary assertion in the hostile-input
test around sanitizeAttachmentFilename: verify a filename longer than 200
characters is sanitized to exactly 200 characters, and verify the 200-character
boundary remains accepted. Preserve the existing non-empty and ADAPTER_SAFE
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eba8f58c-43c4-498a-bac1-cb2b136c1852
📒 Files selected for processing (14)
specs/api/agent-inbox-v1.mdspecs/mail/inbound-ingestion.mdsrc/api/conversations.tssrc/api/index.test.tssrc/api/index.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/store/attachments.test.tssrc/store/attachments.tssrc/store/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/db/postgres.test.ts
- src/store/index.ts
- src/api/index.ts
- src/db/migrate.ts
- src/db/migrate.test.ts
- src/composition/root.ts
- src/store/attachments.test.ts
- src/api/conversations.ts
- src/mail/ingest.ts
- src/store/attachments.ts
- src/api/index.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implemented HT-46: inbound attachment bytes are now persisted to BlobStore instead of being silently dropped. Migration 015 adds a
thread_attachmentstable (thread_id FK cascade, filename nullable, content_type, size, blob_key, created_at) plus asrc/store/attachments.tsmodule (ThreadAttachmentStore.listByConversationId, transaction-scopedinsertThreadAttachmentsInTx).src/mail/ingest.tsnow writes each attachment's bytes to the BlobStore between step 4 (decide) and step 5 (store) under a mailbox-namespaced key<mailboxId>/<attachmentId>/<filename>(attachmentId a fresh UUID, filename sanitized to strip//\), then persists only the blob-key reference inside the same step-5 transaction that writes the thread — so a step-5 abort orphans the already-written blob (tolerable per the ticket's design) and a retry writes a fresh blob rather than repairing the orphan. The loop guard runs before the blob write so a suppressed own-message reflection never writes attachment bytes it would have nothing to reference. On the read side,GET /api/v1/conversations/{id}gained an optional attachment surface:ThreadView.attachments(metadata + aBlobStore.getSignedUrlsigned URL, 1-hour expiry), wired as an absent-by-default dependency onInboxApiDeps(mirroring the existingopenTrackingpattern) so no untouched deployment or test is affected; the composition root wires it for the RIQ dogfood. Updated specs/mail/inbound-ingestion.md (§3's closing paragraph + §8 acceptance bullets) and specs/api/agent-inbox-v1.md (ThreadView/AttachmentView shape, §6, §7 changelog) to document the new behavior. GC for orphaned blobs is explicitly flagged as a follow-up, not built. Fixed one pre-existing test (src/db/postgres.test.ts) that hardcoded the full migrated-table list and neededthread_attachmentsadded.Design decisions
Followed the ticket's design exactly: blob write before the step-5 transaction, reference-only inside it, orphan-tolerant retry. For the read-path (explicitly optional in the ticket), chose the minimal, additive shape:
ThreadView.attachments: AttachmentView[], absent-by-default via an optionalattachments?: { store, blobStore }dependency onInboxApiDeps/handleGetConversation— this is the exact pattern already used foropenTracking,gmailPush,gmailConnectin this codebase, so it required no changes to any existing test or caller (handleReply/handlePostNote's freshly-created threads always reportattachments: []by default parameter, since a brand-new outbound/note thread cannot yet have any). Chose a JOIN-through-threads read query (listByConversationId) over an IN-list/array-param query, sinceSqlValuein this codebase'sDbseam has no array type — the join keeps every attachment for a conversation fetchable in one round trip without widening that seam. Attachment filename sanitization strips/and\only (not full slugification) to keep the key's three-segment shape guaranteed while doing the minimum needed. Signed-URL expiry (3600s) is a reasonable, documented default, not derived from any spec value. Did not touch theweb/UI — this ticket is engine/API-only, and the API addition is backward-compatible (new optional field).Review
0 adversarial findings raised, 0 actionable, all addressed.
Verification
Independent gate: typecheck 0, lint 0, tests 0 (exit codes), clean tree.
Plus implementer evidence: Ran from /Users/tjbaker/Projects/helpthread-worktrees/feat-ht-46-attachment-blob-persistence throughout (after
npm install, which reported "added 137 packages"):npm run typecheck→tsc --noEmit -p tsconfig.json, exit 0, no output (ran twice, both clean).npm run lint→biome check .→ "Checked 182 files in 103ms. No fixes applied." exit 0 (ranlint:fixonce first to auto-format 4 newly-written test/source files, then confirmedlintclean after).npx vitest run src/store/attachments.test.ts src/db/migrate.test.ts→ 27 passed;npx vitest run src/mail/ingest.test.ts→ 18 passed;npx vitest run src/api/index.test.ts→ 98 passed.npm test 2>&1 | tail -200, i.e.vitest run): reported 15 failed / 746 passed / 2 skipped. Investigated: the machine was under extreme concurrent load from sibling agent worktrees also running full test suites simultaneously (uptimeshowed load averages of ~160-170 on this box); 14 of the 15 failures were bareTest timed out in 20000ms/Hook timed out in 10000msacross files I never touched (gmail-oauth, mailbox-tokens, postgres-queue, gmail-watch-state, inbound-deliveries, conversations.ts's own store tests, root.test.ts) — confirmed as pure resource contention, not a regression, by re-runningsrc/db/postgres.test.tsalone with--testTimeout=60000 --hookTimeout=60000: it passed 20/20 once given headroom. The 15th failure was real and mine to fix:src/db/postgres.test.ts's hardcoded full migrated-table-name list didn't include the newthread_attachmentstable — fixed by adding it in alphabetical position (matching that test'sORDER BY table_name).npx vitest run --testTimeout=60000 --hookTimeout=60000 > /tmp/ht46-full-test.log 2>&1; echo "EXIT_CODE=$?" >> /tmp/ht46-full-test.log(captured vitest's own exit code, not a pipe's, per the HT-42 gate-verification lesson). Result:Test Files 42 passed (42),Tests 763 passed (763),EXIT_CODE=0.npm run typecheckandnpm run lintone final time after the postgres.test.ts fix: both exit 0, no findings.git commit;git log --oneline -3shows the new commit588d260on top off69ba48(the branch's base);git status --shortis clean afterward; author email verified as the repo's configured noreply address viagit show HEAD.Link https://resonantiq.atlassian.net/browse/HT-46
🤖 Generated with Claude Code
Summary by CodeRabbit
attachments) when attachment read access is configured; otherwise they return an empty list.